Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(mwlResizeHandle): add resizableContainer input #128

Merged
merged 2 commits into from
Jun 2, 2021
Merged

feat(mwlResizeHandle): add resizableContainer input #128

merged 2 commits into from
Jun 2, 2021

Conversation

cakeinpanic
Copy link
Contributor

Hi!
I have added few changes

  1. Added check for running preventDefault on an event cause it causes errors if project where this directive is used has passive listeners on same event
  2. Added possibility to have handler div not inside of resizable div
    Current architecture demands this structure:
<div mwlResizable>
   <div mwlResizeHandle [resizeEdges]="{bottom: true, right: true}"></div>
</div>

I added possibility to have divs on one level (this is optional)

<div mwlResizable #resizableElement="mwlResizable"></div>
<div mwlResizeHandle [resizableContainer]="resizableElement" [resizeEdges]="{bottom: true, right: true}"></div>

mattlewis92
mattlewis92 reviewed Jun 2, 2021
View reviewed changes
Copy link
Owner

@mattlewis92 mattlewis92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one, thank you for this! Just one small question

src/resizable.directive.ts Outdated
@@ -416,7 +422,7 @@ export class ResizableDirective implements OnInit, OnChanges, OnDestroy {
this.mousemove
).pipe(
tap(({ event }) => {
if (currentResize) {
if (currentResize && event.defaultPrevented) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be !event.defaultPrevented?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I investigated in this topic a bit more and found put that I just blindly copypasted from some stackoverflow 😅
and this field does not actually reflect passive eventlistener

The truth is there is no field to say that, so the only workaround is to add try-catch

mattlewis92
mattlewis92 reviewed Jun 2, 2021
View reviewed changes
src/resize-handle.directive.ts Outdated
@@ -43,9 +53,9 @@ export class ResizeHandleDirective implements OnInit, OnDestroy {

constructor(
private renderer: Renderer2,
private element: ElementRef,
public element: ElementRef,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just made it public for unit tests sake
no real need, just saw same pattern in ResizableDirective and decided to shortcut same way

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK can we keep it private and get the element a different way in the test (via querySelector or debugElement). Users have an annoying way of using all the apis you didn't expect to expose 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally agree, updated

mattlewis92
mattlewis92 approved these changes Jun 2, 2021
View reviewed changes
Copy link
Owner

@mattlewis92 mattlewis92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!! 🙌

@mattlewis92 mattlewis92 changed the title feat: add possibility to declare handle element out of resizable element feat(mwlResizeHandle): add resizableContainer input Jun 2, 2021
@mattlewis92 mattlewis92 merged commit 862249f into mattlewis92:master Jun 2, 2021
@mattlewis92
Copy link
Owner

Released as 3.4.0 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattlewis92 mattlewis92 mattlewis92 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cakeinpanic @mattlewis92